Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BtcSwapTx: update refund tx to consider all utxos #73

Merged

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Sep 13, 2024

This PR updates BtcSwapTx and its new_refund method to consider all available utxos.

Fixes #72

@ok300
Copy link
Contributor Author

ok300 commented Sep 13, 2024

@i5hi as I'm still testing this, I marked it as a draft and wanted to ask for your high level thoughts and feedback:

  • is this the proper way of addressing this, or do you see a better way?
  • should the two BtcSwapTx fields (utxo and utxos) be merged into one?
  • are there any other implications or considerations for this PR that I might have missed?

@i5hi
Copy link
Collaborator

i5hi commented Sep 14, 2024

The approach is good!

We don't need the old utxo field anymore.

@i5hi i5hi self-assigned this Sep 14, 2024
@i5hi i5hi added the enhancement New feature or request label Sep 14, 2024
The BtcSwapTx methods are updated to use all available
inputs, where appropriate.
@ok300
Copy link
Contributor Author

ok300 commented Sep 16, 2024

Alright, I continued with this approach and removed the utxo field in 3587e3c .

Short summary of the changes:

  • removed utxo field
  • updated new_refund to use fetch_utxos instead of fetch_utxo
  • updated sign_claim to rely only on the 1st utxo in utxos (*)
  • updated sign_refund
    • cooperative branch: rely only on the 1st utxo in utxos (*)
    • non-cooperative branch: use all available utxos as inputs

I'm not fully sure about the parts marked with (*), more specifically:

  • Can more than 1 utxos be claimed?
    • I assume no, because as soon as 1 utxo shows up on the lockup address, the Boltz Backend advances the swap to the next state.
  • Can more than 1 utxos be refunded in cooperative mode?
    • I assume no, because this needs interaction with Boltz and intuitively it makes sense to only be able to refund what can be claimed (see above).

What do you think @i5hi? Are my assumptions correct?

@i5hi
Copy link
Collaborator

i5hi commented Sep 16, 2024

  • Can more than 1 utxo be claimed?
    Technically, yes but in practice, No.

Boltz will never lockup more than 1 utxo, so wont need to. However, for consistency the code should attempt to spend all utxos, incase for whatever reason there is more than 1 - no harm I suppose.

  • Can more than 1 utxo be coop refunded?
    Yes.

We refund what we lock* up - so incase we lock up more than 1 utxo, we must refund both.

@i5hi
Copy link
Collaborator

i5hi commented Sep 16, 2024

@michael1011 Need confirmation from you on this

@i5hi
Copy link
Collaborator

i5hi commented Sep 16, 2024

Infact, we should stick with your original approach of usingfetch_utxo for new_claim and always expect and spend just 1 utxo.

while new_refund will use fetch_utxos and always attempt to spend all.

@michael1011
Copy link
Collaborator

michael1011 commented Sep 16, 2024

Can more than 1 utxos be claimed?

In theory, yes. But it's very likely never going to happen (unless there is a grave bug) that we lock more than one UTXO for a swap.

Can more than 1 utxos be refunded in cooperative mode?

Yes. When asking for a cooperative refund signature, one of the parameters is index which is the number of the input you want to have a partial signature for. That way, you can do multiple cooperative refunds (for a single swap or multiple ones) in a single transaction. When we have a swap in a failed state, we are happy to create as many refund signatures as you would like.

src/swaps/bitcoin.rs Outdated Show resolved Hide resolved
src/swaps/bitcoin.rs Outdated Show resolved Hide resolved
src/swaps/bitcoin.rs Outdated Show resolved Hide resolved
This preserves the existing logic of when to fallback to querying Boltz for the utxo.
Add new argument for get_chain_partial_sig, get_submarine_partial_sig that represent the input index.
@ok300
Copy link
Contributor Author

ok300 commented Sep 17, 2024

@i5hi @michael1011 thanks for your feedback. I addressed your points and added one question about sign_refund.

@ok300 ok300 marked this pull request as ready for review September 18, 2024 12:13
@i5hi
Copy link
Collaborator

i5hi commented Sep 19, 2024

@ok300 Have you tried running the integration submarine tests and send 2 payments to the address to see if this all works well?

@i5hi
Copy link
Collaborator

i5hi commented Sep 19, 2024

Also ensure that standard refunds are working as expected

@ok300
Copy link
Contributor Author

ok300 commented Sep 19, 2024

@i5hi I ran cargo test and some tests passed, others were skipped with "ignored, requires invoice and refund address".

Are these the tests you mean and if so, what's the proper way to run them? Just change the invoice and refund address fields, then remove the #ignore annotation?

@i5hi
Copy link
Collaborator

i5hi commented Sep 20, 2024

you dont need to remove ignore, for each submarine test you have to use a fresh invoice or you will get a 400 from boltz:

cargo test --test bitcoin bitcoin_v2_submarine -- --nocapture --ignored 

@ok300
Copy link
Contributor Author

ok300 commented Sep 20, 2024

I tried bitcoin_v2_submarine with a fresh testnet invoice from Phoenix, but running the command you posted failed with "invoice not for current active network: testnet3".

Do I have to set anything else?

@i5hi
Copy link
Collaborator

i5hi commented Sep 20, 2024

Just tried a pheonix testnet invoice; worked fine.

Try this

lntb5125180n1pnwmt4zpp5mvhqku7k4m4smvhdft67qq0kfpsg3jktsp99uaws60rq6m9wumfqcqpjsp5qlw0j3wlpnzx4cmrk3n8f09fqra2fmwjm4jk0j2l8uqeclag8c9q9q7sqqqqqqqqqqqqqqqqqqqsqqqqqysgqdqqmqz9gxqyjw5qrzjqwfn3p9278ttzzpe0e00uhyxhned3j5d9acqak5emwfpflp8z2cnflctr6qq3f9n3gqqqqlgqqqqqeqqjqeh4hcz22hnr9xc0tat79q0aujefdreewpseqvh6z4afk2qyd067r5z8zq863m5lgkj9nkrdmmtalf6j7zejla0xur3e22mq4dtev9tqpkhanae

@ok300
Copy link
Contributor Author

ok300 commented Oct 16, 2024

I ran two kinds of tests:

First, I used this branch to trigger (and complete) refunds for Incoming Chain Swaps (BTC -> LBTC) in breez/breez-sdk-liquid#471 . Both cooperative and non-cooperative claims (to LBTC, as final step of the chain swap) and refunds (to BTC) were successful.

Second, I used @i5hi 's suggestion above to run the bitcoin_v2_submarine test. It essentially worked (the new invoice was paid), even though the test printed an error HTTP("mempool min fee not met, 224 < 4543") at L170, which is when calling boltz_api_v2.post_submarine_claim_tx_details. The testnet mempool fee estimation is generally erratic. I also didn't find a way to set custom (higher) feerates for the claim tx in the bitcoin_v2_submarine test. IMO this is a separate issue related to feerates on testnet.

The PR is ready for final review 🙏

@ok300
Copy link
Contributor Author

ok300 commented Oct 28, 2024

@michael1011 @i5hi anything else I can do to help with the review?

We are using this branch in the Liquid Breez SDK and it would make life easier to get it merged 🙏

Copy link
Collaborator

@michael1011 michael1011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@i5hi i5hi merged commit 8b8bdde into SatoshiPortal:trunk Oct 28, 2024
4 checks passed
@i5hi
Copy link
Collaborator

i5hi commented Oct 28, 2024

Thanks @ok300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incoming chain swaps: refund only considers 1st UTXO
3 participants